Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

libcontainer: support set stdios for container #2961

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

libcontainer: support set stdios for container #2961

wants to merge 1 commit into from
+148 −1

Conversation

abel-von
Copy link

currently the container can only inherit stdios from it's parent process, the one who create container with libcontainer can not set stdios to a different file. this restrict the use of the libcontainer that a process can only create one container by itself, because we can't imagine that every container this process creates share the same stdin/stdout/stderr.

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Oct 25, 2024

May I ask you to take a look at discussion done in #2749 , as it was opened with similar intentions.

@abel-von
Copy link
Author

@YJDoc2 Sorry, I also noticed that there is already a related PR after I submit this PR, but it seems it is a breaking change there, but in this PR, we just add three with_std* functions in the builder, it seems no breaking change here.

@abel-von abel-von mentioned this pull request Oct 26, 2024
Open
jprendes
jprendes reviewed Oct 26, 2024
View reviewed changes
Copy link
Contributor

@jprendes jprendes left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thanks @abel-von.
We are currently doing the same dup2 trick in runwasi here (although we try to keep windows support there) and it works great so far.
I do prefer this over #2749 as it has a more limited scope.

crates/libcontainer/src/container/builder.rs Outdated
@@ -24,6 +25,12 @@ pub struct ContainerBuilder {
/// The function that actually runs on the container init process. Default
/// is to execute the specified command in the oci spec.
pub(super) executor: Box<dyn Executor>,
// RawFd set to stdin of the container init process.
pub stdin: Option<RawFd>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better for this to be OwnedFd.
With the current design as used in the doc example, this is leaking FDs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also hesitated between OwnedFd and RawFd, At last I think maybe RawFd is better because with RawFd the caller has its control over when to close them, otherwise the fd is closed when the it go out of some scope, maybe it is not our expected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With RawFd there's a risk the caller closes the file before we use it.
If they dont want their file closed, and want to retain ownership, the caller can always call try_clone() to obtain a new OwnedFd.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are right, OwnedFd is good, Now The stdin/stdout/stderr in ContainerBuilder is OwnedFd, but in ContainerArgs it is still RawFd because it should implement Clone but OwnedFd does not implement Clone. So I changed it to RawFd with as_raw_fd() when new ContainerArgs, I'm not sure if that is appropriate, please take a look. Thanks

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Oct 28, 2024

Hey @abel-von , before checking further, couple of comments -

  1. CI is failing, there are some typos and some doc comment code is not running properly , can you take a look and fix?
  2. Would it be possible to mark the builder method for setting these fds as unsafe ? Seeing your reasoning for using RawFds, maybe it might be better to set these methods to unsafe if possible, and make sure user knows what they're doing, would that make sense?

@abel-von abel-von force-pushed the support-set-stdio branch 2 times, most recently from 539c48f to 3608a4d Compare October 28, 2024 07:16
@abel-von
Copy link
Author

abel-von commented Oct 28, 2024 •
edited
Loading

Hi @YJDoc2, The CI failure is fixed, could you please trigger the CI again. Thank you.

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Oct 28, 2024

Hey, I think the tests are passing, but the lint and fmt CI is failing again. We use the just commands in CI to validate, so you can also run them locally and check before pushing.

Also I'm wondering, would it be possible to have e2e or unit tests for this feature?

@abel-von
Copy link
Author

Hi @YJDoc2, just lint and just test-all has been run and codes are force pushed again, CI must be ok now. I tried to add a e2e test but it seems there is no e2e tests for libcontainer only, I can only add a unit test in builder.rs to make sure the with_stdin/with_stdout/with_stderr is correctly set.

@YJDoc2
Copy link
Collaborator

YJDoc2 commented Oct 29, 2024

I tried to add a e2e test but it seems there is no e2e tests for libcontainer only, I can only add a unit test in builder.rs to make sure the with_stdin/with_stdout/with_stderr is correctly set.

Ok, I'll think on this. Btw, I have run the CI again, but there are conflicts with main branch due to another PR that was merged. Can you take a look and resolve them? Thanks.

@abel-von
libcontainer: support set stdio for container
7da767b 
currently the container can only inherit stdios from it's parent
process, the one who create container with libcontainer can not set
stdios to a different file.

Signed-off-by: Abel Feng <fshb1988@gmail.com>
@abel-von
Copy link
Author

@YJDoc2 confliction is resolved. some of my just command can not succeed because my own environment. please check again. Thanks

@YJDoc2 YJDoc2 mentioned this pull request Oct 30, 2024
Open
@abel-von abel-von mentioned this pull request Oct 31, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jprendes jprendes jprendes left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
kind/feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@abel-von @YJDoc2 @jprendes